[storage] remove dead retry #25455

Stevenjin8 · 2022-07-28T21:43:11Z

Description

This PR is a little different from #25017 and the question is around the retry mechanism in

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py

Lines 335 to 391 in 33acf79

    
           retry_active = True 
        
           retry_total = 3 
        
           while retry_active: 
        
               try: 
        
                   location_mode, response = await self._clients.blob.download( 
        
                       range=range_header, 
        
                       range_get_content_md5=range_validation, 
        
                       validate_content=self._validate_content, 
        
                       data_stream_total=None, 
        
                       download_stream_current=0, 
        
                       **self._request_options) 
        
                   # Check the location we read from to ensure we use the same one 
        
                   # for subsequent requests. 
        
                   self._location_mode = location_mode 
        
                   # Parse the total file size and adjust the download size if ranges 
        
                   # were specified 
        
                   self._file_size = parse_length_from_content_range(response.properties.content_range) 
        
                   # Remove any extra encryption data size from blob size 
        
                   self._file_size = adjust_blob_size_for_encryption(self._file_size, self._encryption_data) 
        
                   if self._end_range is not None: 
        
                       # Use the length unless it is over the end of the file 
        
                       self.size = min(self._file_size, self._end_range - self._start_range + 1) 
        
                   elif self._start_range is not None: 
        
                       self.size = self._file_size - self._start_range 
        
                   else: 
        
                       self.size = self._file_size 
        
                   retry_active = False 
        
               except HttpResponseError as error: 
        
                   if self._start_range is None and error.response.status_code == 416: 
        
                       # Get range will fail on an empty file. If the user did not 
        
                       # request a range, do a regular get request in order to get 
        
                       # any properties. 
        
                       try: 
        
                           _, response = await self._clients.blob.download( 
        
                               validate_content=self._validate_content, 
        
                               data_stream_total=0, 
        
                               download_stream_current=0, 
        
                               **self._request_options) 
        
                           retry_active = False 
        
                       except HttpResponseError as error: 
        
                           process_storage_error(error) 
        
                       # Set the download size to empty 
        
                       self.size = 0 
        
                       self._file_size = 0 
        
                   else: 
        
                       process_storage_error(error) 
        
               except ClientPayloadError as error: 
        
                   retry_total -= 1 
        
                   if retry_total <= 0: 
        
                       raise ServiceResponseError(error, error=error) 
        
                   await asyncio.sleep(1)

which was added in #18164. Notably, this retry mechanism catches ClientPayloadError. A few months later, #20888 changed AioHttpTransportResponse.load_body to catch ClientPayloadError's and raise them as IncompleteReadError. So right now, the retry mechanism isn't doing anything (also when i remove it, all tests pass).

So far, this is looking a lot like #25017. But the crucial difference lies in where the body of the response is loaded. In #25017, the body of the response was loaded outside the pipeline in the process_content function. For the async clients, the data is loaded inside the pipeline.

The pipeline is defined in

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/base_client_async.py

Lines 92 to 106 in 33acf79

    
           QueueMessagePolicy(), 
        
           config.headers_policy, 
        
           config.proxy_policy, 
        
           config.user_agent_policy, 
        
           StorageContentValidation(), 
        
           StorageRequestHook(**kwargs), 
        
           self._credential_policy, 
        
           ContentDecodePolicy(response_encoding="utf-8"), 
        
           AsyncRedirectPolicy(**kwargs), 
        
           StorageHosts(hosts=self._hosts, **kwargs), # type: ignore 
        
           config.retry_policy, 
        
           config.logging_policy, 
        
           AsyncStorageResponseHook(**kwargs), 
        
           DistributedTracingPolicy(**kwargs), 
        
           HttpLoggingPolicy(**kwargs),

The AsyncStorageResponseHook is after config.retry_policy. If we look at AsyncStorageResponseHook.send, we see

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/policies_async.py

Line 65 in 33acf79

await response.http_response.load_body()

meaning that the body of the response is loaded later in the pipeline than the retry policy. Thus, if loading the body of the response raises a IncompleteReadError, the pipeline will retry the request. If the retry policy runs out of retries, then it will raise a ServiceResponseError.

In other words, if we modify the #18164 's retry mechanism to catch ServiceResponseErrors instead of aiohttp.ClientPayloadErrors, we would be tripling the number of retries as @jalauzon-msft/@vincenttran-msft suggested earlier.

Given that, I think the best way forward is to revert #18164 's retry mechanism because we already have a working retry mechanism and the current code shouldn't ever run.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

…re-sdk-for-python into fix/storage-aiohttp-dependency

azure-sdk · 2022-07-28T22:29:43Z

API change check

API changes are not detected in this pull request.

Stevenjin8 · 2022-07-29T23:40:00Z

currently working on tests. They are only passing when playing live. I think something funky is going on with vcr

Stevenjin8 · 2022-08-02T20:27:56Z

currently working on tests. They are only passing when playing live. I think something funky is going on with vcr

We could also mark the test as live-only.

jalauzon-msft · 2022-08-03T22:54:14Z

Hi @Stevenjin8, thanks for the investigation / explanation about why this retry code is no longer needed. I think you are correct in that the response streaming case does not really come into play here in async since load_body is in the response hook. What really sold it for me was the fact that in the process_content function for async we are just grabbing the repsonse body that has already been loaded:

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py

Line 35 in e789c73

content = data.response.body()

So here we are really only wrapping the calls to _client.download in retry which is not needed since the normal download calls will go through our regular pipeline with retries.

For me, I think there is still a question of whether load_body will actually perform retries if response streaming is taking place. It is in the pipeline, yes, but I'm not sure the response streaming requests will actually go through the pipeline and be retried. We may end up needing to wrap load_body in retries. But this does not affect this PR so we should be good.

sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py

sdk/storage/azure-storage-blob/tests/test_retry_async.py

Steven Jin and others added 19 commits June 23, 2022 15:39

Don't import from aiohttp in blob.aio

23aa10e

update changelog

802de5c

remove dead code

aa70719

changelog formatting

a9d97d6

add retries back in

46efbf9

Catch correct exception

5ba8e98

Merge branch 'main' into fix/storage-aiohttp-dependency

b3f9f54

Update sdk/storage/azure-storage-blob/CHANGELOG.md

ab942ac

missing import

307bd48

Merge branch 'fix/storage-aiohttp-dependency' of github.com:Azure/azu…

c0037d7

…re-sdk-for-python into fix/storage-aiohttp-dependency

catch azure.core exceptions

40f49f5

Revert

f82a0e9

changelog typo

5f340c0

changelog formatting

685e130

more changelog

a3144d0

merge main

d1d7fef

Merge branch 'main' into fix/storage-aiohttp-dependency

789ab08

start retry tests

5fbf043

remove unused retry

6aa0a3a

ghost added the Storage Storage Service (Queues, Blobs, Files) label Jul 28, 2022

Stevenjin8 mentioned this pull request Jul 28, 2022

Revert "[Storage] Revert removing aiohttp dependency for storage.blob.aio" #25393

Closed

6 tasks

Stevenjin8 changed the title ~~Fix/storage aiohttp dependency~~ [storage] remove dead retry Jul 29, 2022

Steven Jin added 3 commits July 29, 2022 16:33

initial tests

9e55db6

remove retry

ec0aef3

Merge branch 'main' into fix/storage-aiohttp-dependency

d9d31e6

Stevenjin8 marked this pull request as ready for review July 29, 2022 23:39

Stevenjin8 requested review from annatisch, jalauzon-msft and vincenttran-msft as code owners July 29, 2022 23:39

typo

80096aa

Stevenjin8 self-assigned this Aug 2, 2022

jalauzon-msft approved these changes Aug 3, 2022

View reviewed changes

sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py Outdated Show resolved Hide resolved

sdk/storage/azure-storage-blob/tests/test_retry_async.py Show resolved Hide resolved

Steven Jin added 2 commits August 4, 2022 08:35

unused im

efd1473

live-only test

f65d1f0

jalauzon-msft merged commit 60b1048 into main Aug 4, 2022

jalauzon-msft deleted the fix/storage-aiohttp-dependency branch August 4, 2022 23:02

wonder6845 pushed a commit to wonder6845/azure-sdk-for-python that referenced this pull request Aug 23, 2022

[Storage] Remove dead retry from async blob download (Azure#25455)

3c801f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[storage] remove dead retry #25455

[storage] remove dead retry #25455

Stevenjin8 commented Jul 28, 2022 •

edited

Loading

azure-sdk commented Jul 28, 2022

Stevenjin8 commented Jul 29, 2022

Stevenjin8 commented Aug 2, 2022

jalauzon-msft commented Aug 3, 2022

	retry_active = True
	retry_total = 3
	while retry_active:
	try:
	location_mode, response = await self._clients.blob.download(
	range=range_header,
	range_get_content_md5=range_validation,
	validate_content=self._validate_content,
	data_stream_total=None,
	download_stream_current=0,
	**self._request_options)

	# Check the location we read from to ensure we use the same one
	# for subsequent requests.
	self._location_mode = location_mode

	# Parse the total file size and adjust the download size if ranges
	# were specified
	self._file_size = parse_length_from_content_range(response.properties.content_range)
	# Remove any extra encryption data size from blob size
	self._file_size = adjust_blob_size_for_encryption(self._file_size, self._encryption_data)

	if self._end_range is not None:
	# Use the length unless it is over the end of the file
	self.size = min(self._file_size, self._end_range - self._start_range + 1)
	elif self._start_range is not None:
	self.size = self._file_size - self._start_range
	else:
	self.size = self._file_size
	retry_active = False

	except HttpResponseError as error:
	if self._start_range is None and error.response.status_code == 416:
	# Get range will fail on an empty file. If the user did not
	# request a range, do a regular get request in order to get
	# any properties.
	try:
	_, response = await self._clients.blob.download(
	validate_content=self._validate_content,
	data_stream_total=0,
	download_stream_current=0,
	**self._request_options)
	retry_active = False
	except HttpResponseError as error:
	process_storage_error(error)

	# Set the download size to empty
	self.size = 0
	self._file_size = 0
	else:
	process_storage_error(error)

	except ClientPayloadError as error:
	retry_total -= 1
	if retry_total <= 0:
	raise ServiceResponseError(error, error=error)
	await asyncio.sleep(1)

	QueueMessagePolicy(),
	config.headers_policy,
	config.proxy_policy,
	config.user_agent_policy,
	StorageContentValidation(),
	StorageRequestHook(**kwargs),
	self._credential_policy,
	ContentDecodePolicy(response_encoding="utf-8"),
	AsyncRedirectPolicy(**kwargs),
	StorageHosts(hosts=self._hosts, **kwargs), # type: ignore
	config.retry_policy,
	config.logging_policy,
	AsyncStorageResponseHook(**kwargs),
	DistributedTracingPolicy(**kwargs),
	HttpLoggingPolicy(**kwargs),

[storage] remove dead retry #25455

[storage] remove dead retry #25455

Conversation

Stevenjin8 commented Jul 28, 2022 • edited Loading

Description

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

azure-sdk commented Jul 28, 2022

Stevenjin8 commented Jul 29, 2022

Stevenjin8 commented Aug 2, 2022

jalauzon-msft commented Aug 3, 2022

Stevenjin8 commented Jul 28, 2022 •

edited

Loading